[IR] Fix vector.splice verifier scaling by vscale for fixed length vectors #170807
Conversation
…ctors Currently we multiply the known minimum number of elements by vscale even if the vector in question is fixed, so sometimes we miss some fixed vectors with out of bounds indices.
|
@llvm/pr-subscribers-llvm-ir Author: Luke Lau (lukel97) ChangesCurrently we multiply the known minimum number of elements by vscale even if the vector in question is fixed, so sometimes we miss some fixed vectors with out of bounds indices. Full diff: https://github.com/llvm/llvm-project/pull/170807.diff 2 Files Affected:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 439b3859fd3ac..10e6c452680b0 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -6570,7 +6570,8 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
VectorType *VecTy = cast<VectorType>(Call.getType());
int64_t Idx = cast<ConstantInt>(Call.getArgOperand(2))->getSExtValue();
int64_t KnownMinNumElements = VecTy->getElementCount().getKnownMinValue();
- if (Call.getParent() && Call.getParent()->getParent()) {
+ if (VecTy->isScalableTy() && Call.getParent() &&
+ Call.getParent()->getParent()) {
AttributeList Attrs = Call.getParent()->getParent()->getAttributes();
if (Attrs.hasFnAttr(Attribute::VScaleRange))
KnownMinNumElements *= Attrs.getFnAttrs().getVScaleRangeMin();
diff --git a/llvm/test/Verifier/invalid-splice.ll b/llvm/test/Verifier/invalid-splice.ll
index 2239386df562f..c34f4d0898aa0 100644
--- a/llvm/test/Verifier/invalid-splice.ll
+++ b/llvm/test/Verifier/invalid-splice.ll
@@ -26,7 +26,7 @@ define <2 x double> @splice_v2f64_idx2(<2 x double> %a, <2 x double> %b) #0 {
; CHECK: The splice index exceeds the range [-VL, VL-1] where VL is the known minimum number of elements in the vector
define <2 x double> @splice_v2f64_idx3(<2 x double> %a, <2 x double> %b) #1 {
- %res = call <2 x double> @llvm.vector.splice.v2f64(<2 x double> %a, <2 x double> %b, i32 4)
+ %res = call <2 x double> @llvm.vector.splice.v2f64(<2 x double> %a, <2 x double> %b, i32 3)
ret <2 x double> %res
}
|
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
LGTM, but the vscale handling also looks wrong to me. Shouldn't it be using the maximum value instead of the minimum, to detect the cases that are "definitely out of range" rather than "maybe out of range"?
The LangRef actually restricts immediates that "may be out of range":
But yeah I think that's overly strict. We can probably relax that to -X <= imm < X where X=vscale_range_max * N? And then copy over the wording from e.g. llvm.vector.extract
|
|
As you are planning to extend this to dynamic indices, the verifier check must be dropped at that point anyway (with poison semantics for the actual runtime value), so we can just defer this until then... (We can't have verifier checks only for the constant case.) |
…ctors (llvm#170807) Currently we multiply the known minimum number of elements by vscale even if the vector in question is fixed, so sometimes we miss some fixed vectors with out of bounds indices.
3 participants
Currently we multiply the known minimum number of elements by vscale even if the vector in question is fixed, so sometimes we miss some fixed vectors with out of bounds indices.